-
Notifications
You must be signed in to change notification settings - Fork 44
Conversation
src/prompt.ts
Outdated
setTimeout(() => reject(), options.timeout || 10000).unref() | ||
setTimeout(() => { | ||
process.stdin.end() | ||
reject(new Error('Prompt timeout')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not having an error object in reject was causing issues later in oclif/config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that's something TypeScript should enforce (possibly behind a flag)
src/prompt.ts
Outdated
@@ -81,6 +81,9 @@ function normal(options: IPromptConfig, retries = 100): Promise<string> { | |||
resolve(data || options.default) | |||
} | |||
}) | |||
setTimeout(() => reject(), options.timeout || 10000).unref() | |||
setTimeout(() => { | |||
process.stdin.end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing piece. 🍕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would prevent us from reading stdin ever again in this process which seems problematic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would process.stdin.pause()
work?
Codecov Report
@@ Coverage Diff @@
## master #36 +/- ##
=========================================
Coverage ? 46.26%
=========================================
Files ? 13
Lines ? 335
Branches ? 72
=========================================
Hits ? 155
Misses ? 153
Partials ? 27
Continue to review full report at Codecov.
|
src/prompt.ts
Outdated
@@ -81,8 +81,9 @@ function normal(options: IPromptConfig, retries = 100): Promise<string> { | |||
resolve(data || options.default) | |||
} | |||
}) | |||
if (options.timeout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you removed this conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it in to fix this issue: heroku/cli#880
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge mistake, putting it back
src/prompt.ts
Outdated
setTimeout(() => { | ||
process.stdin.pause() | ||
reject(new Error('Prompt timeout')) | ||
}, options.timeout || 10000).unref() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to default with the conditional in place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
<a name="4.6.3"></a> ## [4.6.3](v4.6.2...v4.6.3) (2018-06-12) ### Bug Fixes * pause prompt stdin ([#36](#36)) ([1443998](1443998))
🎉 This PR is included in version 4.6.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.